-
-
Notifications
You must be signed in to change notification settings - Fork 906
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add commit co-authors support #1482
Conversation
… message for designated co-authors, include a simple test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, this looks good already!
I have some notes, but when clarified it should be ready for merging.
… the _from_string classmethod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
The only remaining question pertains to the regex used, but once clarified this is ready to go.
Maybe you could also fix flake8 by ignoring this new check that makes CI fail, if you feel comfortable.
git/objects/commit.py
Outdated
@@ -752,11 +752,11 @@ def co_authors(self) -> List[Actor]: | |||
|
|||
if self.message: | |||
results = re.findall( | |||
r"^Co-authored-by: ((?:\w|\-| ){0,38}) <(\S*)>$", | |||
r"^Co-authored-by: ((?:\w|\-| ){0,38} <\S*>)$", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, can you explain where this regex is coming from? Earlier I tried to argue that the one used by Actor
could be used here. It's much more open than this, but I can be swayed if there is a specification of the Co-authored-by:
field that limits it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my bad! Regarding the name part, this is on me, I used a regex to match against the GitHub username specs, but this is just the account name, so a more generic one is necessary - switched to match the regex used in Actor
(with the extra Co-authored-by:
text and start/end line checks).
Also added a line to ignore the flake8 check - not used flake8 but I think this should be right.
Thanks a lot for your contribution! |
Add co_authors property to the Commit object, which parses the commit message for designated co-authors, include a simple test.